-
Notifications
You must be signed in to change notification settings - Fork 806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService #4636
Merged
mergify
merged 4 commits into
spinnaker:master
from
Pranav-b-7:remove-retrofitError-KeelService
Feb 27, 2024
Merged
refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService #4636
mergify
merged 4 commits into
spinnaker:master
from
Pranav-b-7:remove-retrofitError-KeelService
Feb 27, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
from
January 29, 2024 16:46
5599618
to
2c86134
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
from
January 30, 2024 13:33
2c86134
to
f6cef42
Compare
dbyron-sf
reviewed
Jan 30, 2024
orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
Outdated
Show resolved
Hide resolved
dbyron-sf
reviewed
Jan 30, 2024
orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
Outdated
Show resolved
Hide resolved
dbyron-sf
reviewed
Jan 30, 2024
orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
Outdated
Show resolved
Hide resolved
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
3 times, most recently
from
February 1, 2024 11:34
ad23108
to
b0bde12
Compare
dbyron-sf
reviewed
Feb 1, 2024
orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java
Show resolved
Hide resolved
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
17 times, most recently
from
February 6, 2024 16:44
5cff4f7
to
8f3521d
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
9 times, most recently
from
February 26, 2024 14:21
15b8fbc
to
0fca8b2
Compare
dbyron-sf
reviewed
Feb 26, 2024
orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
Outdated
Show resolved
Hide resolved
dbyron-sf
reviewed
Feb 26, 2024
orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java
Outdated
Show resolved
Hide resolved
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
6 times, most recently
from
February 27, 2024 11:21
76ab1a0
to
b85fb76
Compare
across different time units when any 4xx http error has occured. These tests ensures the upcoming changes on KeelService with SpinnakerRetrofitErrorHandler, will not modify/break the existing functionality. Tests covers positive and negative cases with different units of timestamps. In addition, another test added to verify when the error response body doesn't have timestamp field.
…errors and network errors Test cases added to verify the upcoming changes when KeelService is configured with SpinnakerRetrofitErrorHandler. These tests verifies the behaviour of the method handleRetryableFailures() when 4xx with empty error body and 5xx http erros are thrown, and also during network errors. NOTE : These tests only verifies the scenario when the error response body is empty/null, and network errors.
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
from
February 27, 2024 11:44
6d9cea3
to
6320f01
Compare
The deleted tests are the cases/scenarios which are already covered as part of the commit: 5d711ab. These tests covers up the http 4xx error cases of the API : keelService.publishDeliveryConfig. Tests deleted are : 1. 'keel access denied error', 2. 'delivery config parsing error'.
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService The tests modified as part of this PR will verify the new changes with the scenarios:- Reading the Http error response body and building the TaskResult by instantiating SpringHttpError. Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body. - On any 4xx http errors with empty error body: before: 11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request} after: 12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://localhost:62275/delivery-configs/, Message: Bad Request} - On any 5xx http errors with empty error body: before: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={}) after: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={})
Pranav-b-7
force-pushed
the
remove-retrofitError-KeelService
branch
from
February 27, 2024 11:47
6320f01
to
c417922
Compare
dbyron-sf
approved these changes
Feb 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
auto merged
Merged automatically by a bot
ready to merge
Approved and ready for merge
target-release/1.34
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService.
Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body.
On any 4xx http errors with empty error body:
before:
11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request}
after:
12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://localhost:62275/delivery-configs/, Message: Bad Request}
On any 5xx http errors with empty error body:
before:
TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={})
after:
TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={})
Note : The RetrofitError catch block will still be relevant even after the code changes in the below classes, due to the reasons mentioned inline: